Hive: Introduce HiveMetastoreExtension for Hive tests by nk1506 · Pull Request #9282 · apache/iceberg

nk1506 · 2023-12-12T06:43:10Z

CC: @nastra

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableBaseTest.java

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommitLocks.java

hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java

nastra

thanks @nk1506, this LGTM

ajantha-bhat · 2023-12-12T10:30:23Z

Let me take a look

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java

ajantha-bhat · 2023-12-12T10:40:39Z

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java

+
  @BeforeEach
  public void createTableLocation() throws IOException {
+    catalog =


I think we should initialize the catalog only once. Not for each tests as nothing changes between tests related to catalog config.

Also can we move this to HiveMetastoreExtension and get the catalog from there? It can help in duplicating this code in each test class which uses this extension.

As consistency with other catalog tests which initialise before each test and drop the catalog after every test. For Hive also the idea is same.

Also can we move this to HiveMetastoreExtension and get the catalog from there? It can help in duplicating this code in each test class which uses this extension.

I don't think the catalog initialization should be moved into HiveMetastoreExtension. The sole purpose of HiveMetastoreExtension is to provide the metastore part

@nastra: Thoughts?

I've replied already above

Sorry, I wanted your thoughts on this comment
#9282 (comment)

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java

ajantha-bhat · 2023-12-12T10:47:10Z

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java

+
  @BeforeEach
  public void createTableLocation() throws IOException {
+    catalog =


Also can we move this to HiveMetastoreExtension and get the catalog from there? It can help in duplicating this code in each test class which uses this extension.

ajantha-bhat · 2023-12-12T10:50:12Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java

            CachedClientPool.clientPoolCache()
-                .getIfPresent(CachedClientPool.extractKey(null, hiveConf)))
+                .getIfPresent(
+                    CachedClientPool.extractKey(null, HIVE_METASTORE_EXTENSION.hiveConf())))


nit: some test we are extracting to a variable and in some test we are using directly. Let us have a unified style.

At other test hiveConf was being used at multiple places, that's the reason I created a local variable.

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommitLocks.java

ajantha-bhat

Now it is LGTM.

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>

Hive: Use jUnit5 based HiveMetastoreExtension with HiveTests

51cea89

github-actions bot added the hive label Dec 12, 2023

nk1506 mentioned this pull request Dec 12, 2023

Hive: Refactor TestHiveCatalog tests to use the core CatalogTests #8918

Merged

nastra reviewed Dec 12, 2023

View reviewed changes

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated Show resolved Hide resolved

nastra requested changes Dec 12, 2023

View reviewed changes

nk1506 force-pushed the hive_junit5_tests branch from 681ede8 to c59e4d3 Compare December 12, 2023 08:44

nastra reviewed Dec 12, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java Outdated Show resolved Hide resolved

nastra reviewed Dec 12, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java Outdated Show resolved Hide resolved

nk1506 force-pushed the hive_junit5_tests branch from c59e4d3 to 13fde54 Compare December 12, 2023 09:19

nastra changed the title ~~Hive: Use jUnit5 based HiveMetastoreExtension with Hive catalog tests~~ Hive: Introduce HiveMetastoreExtension for Hive tests Dec 12, 2023

nk1506 force-pushed the hive_junit5_tests branch from 13fde54 to cde7dd8 Compare December 12, 2023 09:26

nastra approved these changes Dec 12, 2023

View reviewed changes

ajantha-bhat self-requested a review December 12, 2023 10:30

ajantha-bhat reviewed Dec 12, 2023

View reviewed changes

Hive: Remove HiveMetastoeTest with jUnit5 based HiveMetastoreExtension

d1f48bf

nk1506 force-pushed the hive_junit5_tests branch from cde7dd8 to d1f48bf Compare December 12, 2023 11:18

nastra force-pushed the hive_junit5_tests branch from 1825954 to 521fde4 Compare December 12, 2023 12:32

improvements

c81e2b3

nastra force-pushed the hive_junit5_tests branch from 521fde4 to c81e2b3 Compare December 12, 2023 12:32

nastra approved these changes Dec 12, 2023

View reviewed changes

ajantha-bhat approved these changes Dec 12, 2023

View reviewed changes

nastra merged commit 09b44bb into apache:main Dec 12, 2023

lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024

Hive: Introduce HiveMetastoreExtension for Hive tests (apache#9282)

7285847

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>

geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024

Hive: Introduce HiveMetastoreExtension for Hive tests (apache#9282)

460a767

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>

ajantha-bhat mentioned this pull request Mar 26, 2024

Build: Bump Iceberg version to 1.5.2 projectnessie/iceberg-catalog-migrator#148

Merged

devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024

Hive: Introduce HiveMetastoreExtension for Hive tests (apache#9282)

ebc286a

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>

Conversation

nk1506 commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat commented Dec 12, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ajantha-bhat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nk1506 commented Dec 12, 2023 •

edited

Loading